Skip to content

Conversation

@Mangaal
Copy link
Contributor

@Mangaal Mangaal commented Jun 19, 2025

Description

This PR introduces a more secure method for providing Redis credentials to Argo CD components by allowing them to be loaded from a specified file path. Currently, Redis credentials (password, username, sentinel credentials) are configured via environment variables (e.g., REDIS_PASSWORD). Storing secrets in environment variables is a common practice but can be less secure than using file-based secrets.

This change is backward-compatible. Existing setups using environment variables will continue to work without any modification

Proposed Change
This PR introduces a new mechanism to load Redis credentials from files:

  1. New Environment Variable: A new environment variable, REDIS_CREDS_FILE_PATH, is introduced. This variable should point to a directory where credential files are mounted.
  2. File-Based Credential Loading: When REDIS_CREDS_FILE_PATH is set, Argo CD will attempt to read credentials from the following files within that directory:
    • auth: The password for the main Redis connection.
    • auth_username: The username for the main Redis connection.
    • sentinel_auth: The password for Redis Sentinel connections.
    • sentinel_username: The username for Redis Sentinel connections.
  3. Fallback Logic: The implementation prioritises credentials loaded from the file path. If REDIS_CREDS_FILE_PATH is not set, or if a specific credential file does not exist, the system gracefully falls back to using the corresponding environment variables (REDIS_PASSWORD, REDIS_USERNAME, etc.). This ensures full backward compatibility.

Implementation Details

  1. A new struct redisCreds was added to hold the set of credentials.
  2. A new function loadRedisCredsFromFile(mountPath string) was created to handle the logic of reading the individual credential files from the specified mountPath. It is designed to be resilient to missing files, returning empty strings for any credential that cannot be read.
  3. The AddCacheFlagsToCmd function has been updated to incorporate this new logic. It first checks for the REDIS_CREDS_FILE_PATH environment variable. If present, it calls loadRedisCredsFromFile and then checks if any credentials still need to be populated from the environment variables.

How to Test This Change
Mount the Secret into your Argo CD pods (e.g., argocd-repo-server, argocd-application-controller) and set the new environment variable.

spec:
  template:
    spec:
      containers:
      - name: your-argocd-container
        env:
        - name: REDIS_CREDS_FILE_PATH
          value: "/etc/redis-creds"
        # IMPORTANT: Ensure the old env vars like REDIS_PASSWORD are NOT set
        # to verify the new mechanism is working.
        volumeMounts:
        - name: redis-creds-volume
          mountPath: "/etc/redis-creds"
          readOnly: true
      volumes:
      - name: redis-creds-volume
        secret:
          secretName: argocd-redis-creds

Related Issue

Fixes #20619

@Mangaal Mangaal requested a review from a team as a code owner June 19, 2025 11:15
@bunnyshell
Copy link

bunnyshell bot commented Jun 19, 2025

❌ Preview Environment deleted from Bunnyshell

Available commands (reply to this comment):

  • 🚀 /bns:deploy to deploy the environment

@Mangaal Mangaal changed the title feat(redis): Allow loading credentials from a mounted file path (wip)feat(redis): Allow loading credentials from a mounted file path Jun 19, 2025
@blakepettersson blakepettersson changed the title (wip)feat(redis): Allow loading credentials from a mounted file path feat(redis): Allow loading credentials from a mounted file path (wip) Jun 19, 2025
@Mangaal Mangaal changed the title feat(redis): Allow loading credentials from a mounted file path (wip) feat(redis): Allow loading credentials from a mounted file path Jun 19, 2025
Copy link
Member

@anandf anandf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@codecov
Copy link

codecov bot commented Jun 19, 2025

Codecov Report

❌ Patch coverage is 55.10204% with 22 lines in your changes missing coverage. Please review.
✅ Project coverage is 60.36%. Comparing base (12d3f5d) to head (f62ac71).

Files with missing lines Patch % Lines
util/cache/cache.go 55.10% 17 Missing and 5 partials ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master   #23487   +/-   ##
=======================================
  Coverage   60.36%   60.36%           
=======================================
  Files         350      350           
  Lines       60036    60070   +34     
=======================================
+ Hits        36242    36264   +22     
- Misses      20877    20894   +17     
+ Partials     2917     2912    -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@ishitasequeira ishitasequeira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall code looks good!! Can we add some documentation about the feature?

@Mangaal Mangaal requested a review from a team as a code owner June 30, 2025 05:45
@Mangaal Mangaal requested a review from ishitasequeira June 30, 2025 05:51
@Mangaal
Copy link
Contributor Author

Mangaal commented Jun 30, 2025

@ishitasequeira , I have added documentation for the new Redis credentials file-mount feature in the FAQ section.
Please have a look and let me know if any changes are needed.

@Mangaal Mangaal requested a review from nitishfy June 30, 2025 09:47

| Variable Name | Description |
|-------------------------|-----------------------------------------------|
| `REDIS_CREDS_FILE_PATH` | Path to the directory containing credential files |
Copy link
Member

@agaudreault agaudreault Jul 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR should update the default kustomize manifests in manifests/ to use that variable and mount the argocd-redis secret

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have updated the default kustomize manifests as well as the ha.

@Mangaal Mangaal requested a review from agaudreault July 16, 2025 12:07
@nmirasch
Copy link
Contributor

LGTM

dependabot bot and others added 11 commits September 10, 2025 14:22
argoproj#23774)

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: Mangaal <[email protected]>
Signed-off-by: Mangaal <[email protected]>
…st/container (argoproj#23800)

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: Mangaal <[email protected]>
Signed-off-by: Mangaal <[email protected]>
Aamir017 and others added 20 commits September 10, 2025 14:32
…goproj#24445)

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: Mangaal <[email protected]>
…4443)

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: Mangaal <[email protected]>
…j#24413)

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: Mangaal <[email protected]>
…1.23.2 (argoproj#24442)

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: Mangaal <[email protected]>
…4441)

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: Mangaal <[email protected]>
Signed-off-by: CI <[email protected]>
Co-authored-by: CI <[email protected]>
Signed-off-by: Mangaal <[email protected]>
…appset (argoproj#23900)

Signed-off-by: nitishfy <[email protected]>
Co-authored-by: Alexandre Gaudreault <[email protected]>
Signed-off-by: Mangaal <[email protected]>
Signed-off-by: Peter Jiang <[email protected]>
Signed-off-by: Mangaal <[email protected]>
…#24444)

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: Mangaal <[email protected]>
…4472)

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: Mangaal <[email protected]>
@Mangaal Mangaal force-pushed the secrets-via-volume-mount branch from 174b4a3 to eb7c25c Compare September 10, 2025 09:03
@Mangaal Mangaal requested a review from a team as a code owner September 10, 2025 09:03
@Mangaal
Copy link
Contributor Author

Mangaal commented Sep 17, 2025

Closing this PR due to rebase issues. I’ve created a new PR with the same fix here: [#24597]. Please continue the discussion/review there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Read Secrets from Disk as well as from Env